Feat/segmentation service -> master #116

yzhou-bcom · 2025-11-06T15:17:59Z

No description provided.

…n API

…ions to ensure backward compatibility

jim-bcom · 2025-11-07T10:04:58Z

interfaces/api/pipeline/IImageSegmentationPipeline.h

+namespace api {
+namespace pipeline {
+
+/**


Use this syntax, otherwise doxygen does not generate the documentation properly:

/// Status of image segmentation processing enum class ImageSegmentationStatus { UNINITIALIZED, ///< processing not initialized INITIALIZED, ///< processing correctly initialized, but not started IN_PROGRESS, ///< processing in progress COMPLETED, ///< processing completed ABORTED ///< processing aborted before completion };

If you want to test, you'll also need this change I already merged to master:
d5b6492

Without it, enum declared in undocumented namespace are simply not visible in the documentation.

jim-bcom · 2025-11-07T10:28:18Z

interfaces/api/pipeline/IImageSegmentationPipeline.h

+ * @enum class ImageSegmentationStatus
+ */
+enum class ImageSegmentationStatus {
+    UNINITIALIZED = 0,    // processing not initialized


Also, no need to specify each value I think, because it's probably their default value anyway.

Plus we don't really use them.

jim-bcom · 2025-11-07T10:55:03Z

interfaces/api/pipeline/IImageSegmentationPipeline.h

+    /// @brief segmentation request from an input image
+    /// @param[in] image pointer to image
+    /// @return FrameworkReturnCode::_SUCCESS (segmentation succeeded) or FrameworkReturnCode::_ERROR_ (segmentation failed)
+    virtual FrameworkReturnCode segmentationRequest(SRef<Image> image) = 0;


nit:

I know it's been done elsewhere, but not a fan of this name:

usually a verb is used for method (action). Here it sounds like an object

request is redundant. If we call a method, we make a request to the object to perform an action.

It could be for example: segment(), run/performSegmentation(), or something else.

If we want to be concise, it could even be just run()-, because calling IImageSegmentationPipeline::run() it is clear enough that run() will perform an image segmentation.

Also, I'm not sure, but enum ImageSegmentationStatut could be defined inside the class, and simply be called Status. The method getStatus() is not called getImageSegmentationStatus().

Just some thoughts, you decide ;)

jim-bcom · 2025-11-07T13:02:43Z

interfaces/api/storage/IMasks2DManager.h

@@ -0,0 +1,120 @@
+/**
+ * @copyright Copyright (c) 2017-2025 B-com http://www.b-com.com/


New file => 2025 only, no?

jim-bcom · 2025-11-07T13:19:29Z

interfaces/datastructure/Mask2D.h

+namespace datastructure {
+
+/**
+ * @enum class Segmentation2DType


See other comment about doxygen syntaxt for enums.

jim-bcom · 2025-11-07T14:37:50Z

src/datastructure/Mask2DCollection.cpp

+    return FrameworkReturnCode::_SUCCESS;
+}
+
+bool Mask2DCollection::isExistMask(uint32_t id) const


isExist is not really english. contains()?

jim-bcom · 2025-11-07T16:46:29Z

interfaces/datastructure/Mask2D.h

+        }
+    };
+    using MaskInfoType = std::map<uint8_t, SegInfo>;
+    using ClassLabelType = std::map<int16_t, std::string>;


Shouldn't this table be moved out of Mask2D? I mean, for a given segmentation process, this map is going to be the same, right ?

Or maybe not? Is this a per image thing? But in theory, it could be interesting to use a single table for all masks to not have to repeat this association int->Class in every Mask2D?

MaskInfoType is probably tied to the Mask, because being limited to 255 values, it couldn't encode a whole image sequence, so this makes more sense.

It can stay as is for now I guess, but moving it outside could be an optimization to save some memory, if need be.

the map from class to label is global. for example if the model is trained with 150 classes, it is a map(int -> string) of size 150. currently a map only has 1 mask collection. the idea was to put all segmented masks in one collection which can be a combination of several segmentation models. maybe a map should have a list of mask collections (std::vector<SRef>)

jim-bcom · 2025-11-12T09:44:31Z

interfaces/datastructure/Mask2D.h

+
+    /// @brief set Id
+    /// @param[in] id Mask2D ID
+    void setId(uint32_t id);


Are setters useful if the object can be entirely build with its ctor?

jim-bcom · 2025-11-12T09:54:17Z

src/datastructure/Map.cpp

    return true;
 }

+SRef<const Mask2DCollection> Map::getConstMask2DCollection() const


Is it mandatory to return an SRef? Or a simple ref would suffice?

I.e. is there a scenario where the caller will claim the ownership of the collection, so that it can work on it while the Map is destroyed? Or is the collection lifetime linked to the one of the Map?

the mask collection's lifetime is not linked to one map. user can do getMask2DCollection(SRef& maskCollection) and continue to work on it even when the map had already been destroyed

return a simple ref is a good idea

I'm confused by your comment. You say that the collection can outlive the Map, but you still agree to return a simple ref? If so, the collection cannot be processed after the Map is gone. Maybe you were describing the state as it were, and not how it was intended to be?

jim-bcom · 2025-11-12T10:03:16Z

src/datastructure/Mask2DCollection.cpp

+    if (defineMaskId) {
+        newMask->setId(m_currentId++);
+    }
+    m_masks[newMask->getId()] = newMask;


Thus do you think it's worth checking that the id does not already exist? Is it a warning or an error? Probably an error no?

jim-bcom · 2025-11-17T08:51:10Z

interfaces/datastructure/Mask2DCollection.h

+    /// @brief This method allow to add a Mask2D to the Mask2D manager component
+    /// @param[in] mask the Mask2D to add to the set of persistent Mask2Ds
+    /// @return FrameworkReturnCode::_SUCCESS_ if the addition succeed, else FrameworkReturnCode::_ERROR.
+    FrameworkReturnCode addMask(SRef<Mask2D> mask);


bool defineMaskId = true parameter removed here, but not from IMapManager and IMask2DManager \ addMask(), is that intended?

The idea is to simplify the id management of masks. We let the user to define the ID before adding the mask into the collection. user should do mask->setId() before adding it into the collection. Then, the maskCollection will check if a mask with the same ID exists already.

I don't understand your answer: if the user sets the ID, then we don't need the parameter. So you removed it from here, OK.
My remark was that this parameter was still present in MapManager and Mask2DManager version of addMask()

From what I see currently, MapManager passes defineId to Mask2DManager.addMask(), but then this parameter is ignored by Mask2DManager.addMask(). So maybe it needs to be removed from those classes as well?

As a side note, shouldn't the MaskCollection be responsible for managing the IDs? This way we wouldn't have to check it is valid? You could have MaskCollection::createMask2D() factory method for example.

Or having the addMask2D method return the ID the Collection have assigned to it. But the factory method would guarantee we don't expose a setId() method that allows anyone to change the ID.

Or another solution, don't put ID in Mask2D, and let the Collection manage the ID as the key of its internal map.

jim-bcom · 2025-11-17T08:59:30Z

src/datastructure/Mask2D.cpp

+    return m_maskInfo;
+}
+
+void Mask2D::print() const


nit: maybe you could make a toString() that outputs the JSON, so you can reuse the same impl to print it here, and to output it in the file ?

jim-bcom · 2025-11-17T09:05:09Z

src/datastructure/Mask2DCollection.cpp

+        return FrameworkReturnCode::_ERROR_;
+    }
+    minId = m_masks.begin()->first;
+    maxId = (--m_masks.end())->first;


Nice one. I found this version for max, I'll let you pick the one you prefer (and if it works :p). There's just less syntax:
maxId = m_masks.rbegin()->first;

jim-bcom · 2025-11-17T09:11:42Z

interfaces/datastructure/Mask2D.h

+     * * instanceId, the instance Id of the detected object, if instanceId < 0, it means that the current pixel belongs to a "stuff" class which is uncountable, otherwise it belongs to a "thing" class
+     * * confidence, confidence score between 0 and 1, the confidence score of the segmentation, if confidence < 0, it means that the confidence score is not available
+     */
+    struct SegInfo {


Keep it as is for now, this PR has to move on, but I wonder whether it could be worth introducing some kind of type hierarchy here for different type of segmentation, so that we don't have these kind of struct with fields that are sometimes irrelevant. It's probably better to have things more precisely typed.

…n etc

yzhou-bcom added 12 commits October 15, 2025 17:08

feat: add interface IImageSegmentationPipeline

a28e0c2

Merge branch 'master' into feat/segmentation-service

2470902

feat(seg): add enum struct to IImageSegmentationPipeline

37e83cb

feat: add new data structures Mask2D and Mask2DCollection

cb8430c

fix(seg): update image segmentation pipeline APIs

ace01cd

fix(seg): use SRef<Image> instead of SRef<const Image> in segmentatio…

f290234

…n API

feat(seg): add IMasks2DManager

314c703

Merge 'master' into feat/segmentation-service

e29d409

feat(seg): add print function in Mask2D

4cc2430

fix(serialization): serialize Frame according to different class vers…

156e091

…ions to ensure backward compatibility

feat(seg): export Mask2D to more visualizable formats (png, json)

0c9c055

fix(seg): serialize 2D masks in Map when version is above 0

95d745b

yzhou-bcom requested review from cutullic and jim-bcom November 6, 2025 15:17

jim-bcom reviewed Nov 12, 2025

View reviewed changes

yzhou-bcom added 2 commits November 14, 2025 15:06

refactor(mask2d): move segType and classLabels to Mask2DCollection etc

db547a1

Merge branch 'master' into feat/segmentation-service

2fd2a11

jim-bcom reviewed Nov 17, 2025

View reviewed changes

yzhou-bcom added 2 commits November 17, 2025 17:27

refactor(mask2d): add func toString in Mask2D, return SRef, use rbegi…

56131ef

…n etc

fix(mask2d): move out id from Mask2D to Mask2DCollection

1243dad

		@@ -0,0 +1,120 @@
		/**
		* @copyright Copyright (c) 2017-2025 B-com http://www.b-com.com/

Feat/segmentation service -> master #116

Are you sure you want to change the base?

Feat/segmentation service -> master #116

Uh oh!

Conversation

yzhou-bcom commented Nov 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yzhou-bcom Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yzhou-bcom Nov 17, 2025 •

edited

Loading